-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to babbage era .. and all kinds of stuff #408
Conversation
txOutAddress | ||
(TxOutValue MultiAssetInBabbageEra txOutValue) | ||
txOutDatum | ||
_txReferenceScript |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add FIXME here to not forget about it.
-- NOTE: Requires the 'Network' discriminator (Testnet or Mainnet) because | ||
-- Plutus addresses are stripped off it. | ||
fromPlutusAddress :: IsShelleyBasedEra era => Network -> Plutus.Address -> AddressInEra era | ||
fromPlutusAddress network plutusAddress = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: unclear why we need that yet, let's see if we can clarify later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's from observing commits, there we need to convert from a Plutus.TxOut -> CardanoApi.TxOut ctx era
-- 'TxOutSource' to report corresponding 'TranslationError'. However, this | ||
-- value is NOT used for constructing the Plutus.TxOut and hence we hard-code | ||
-- it to 0 to not need to bookkeep it and we throw away errors anyway. | ||
eitherToMaybe . Ledger.txInfoOutV2 (TxOutFromOutput $ Ledger.TxIx 0) . toLedgerTxOut |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hard-coding a TxIx, we could use a error "TxOutSource" used but shouldn't
was we do not expect this to ever be used / evaluated (because of eitherToMaybe
)
@@ -86,9 +86,9 @@ queryProtocolParameters :: NetworkId -> FilePath -> QueryPoint -> IO ProtocolPar | |||
queryProtocolParameters networkId socket queryPoint = | |||
let query = | |||
QueryInEra | |||
AlonzoEraInCardanoMode | |||
BabbageEraInCardanoMode |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that those queries can only run in Babbage era. Ideally, we need queries to be able to work on both the current era and the next era. If this isn't something that runQuery
does (or some other function from cardano-api
) we should extend them to enable that.
An example of such thing can be found here: https://github.com/CardanoSolutions/ogmios/blob/master/server/src/Ogmios/App/Protocol/TxSubmission.hs#L347
@@ -324,7 +324,7 @@ close confirmedSnapshot pointInTime OnChainHeadState{ownVerificationKey, stateMa | |||
ConfirmedSnapshot{snapshot = Snapshot{number, utxo}, signatures} -> | |||
CloseWithConfirmedSnapshot | |||
{ snapshotNumber = number | |||
, closeUtxoHash = hashTxOuts $ toList utxo | |||
, closeUtxoHash = hashUTxO @Tx utxo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if these are indeed semantically equivalent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should be. I will change back the definition of hashUTxO
to be not sorting besides using toList
.
@@ -388,7 +382,7 @@ contestTx vk Snapshot{number, utxo} sig (slotNo, _) ClosedThreadOutput{closedThr | |||
, parties = closedParties | |||
, contestationDeadline = closedContestationDeadline | |||
} | |||
utxoHash = toBuiltin $ hashTxOuts $ toList utxo | |||
utxoHash = Head.hashTxOuts . mapMaybe toPlutusTxOut $ toList utxo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that use the more generic 'hashUTxO' ?
headRedeemer = | ||
toScriptData (Head.Fanout $ fromIntegral $ length utxo) | ||
|
||
headTokens = | ||
headTokensFromValue headTokenScript (txOutValue headOutput) | ||
|
||
fanoutOutputs = | ||
map toTxContext $ toList utxo | ||
map (toTxContext . snd) . sortOn fst $ UTxO.pairs utxo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is in principle semantically equivalent, because toList
returns elements ordered by key already. So perhaps we could make the naming clearer or add a comment to clarify this.
@@ -192,15 +166,16 @@ newTinyWallet tracer networkId (vk, sk) queryUTxOEtc = do | |||
-- checking the output's address. | |||
applyBlock :: Block -> (Address -> Bool) -> Map TxIn TxOut -> Map TxIn TxOut | |||
applyBlock blk isOurs utxo = case blk of | |||
BlockAlonzo (ShelleyBlock (Ledger.Block _ bbody) _) -> | |||
BlockBabbage (ShelleyBlock block _) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as queries, we probably want to support 2 eras always, the current and next one.
let outs = outputs (body tx) | ||
in StrictSeq.zip (StrictSeq.fromList [0 .. length outs]) outs | ||
forM_ indexedOutputs $ \(fromIntegral -> ix, out@(TxOut addr _ _)) -> | ||
let outs = map sizedValue . toList $ outputs (body tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use outputs'
or getField @"outputs"
to avoid dealing with sized value.
@@ -262,14 +237,14 @@ coverFee_ pparams systemStart epochInfo lookupUTxO walletUTxO partialTx@Validate | |||
mkChange | |||
output | |||
resolvedInputs | |||
(toList $ outputs body) | |||
(map sizedValue . toList $ outputs body) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here -> outputs'
@@ -122,7 +114,7 @@ instance IsTx Tx where | |||
|
|||
txId = getTxId . getTxBody | |||
balance = foldMap txOutValue | |||
hashUTxO = hashTxOuts . toList | |||
hashUTxO = fromBuiltin . Head.hashTxOuts . mapMaybe (toPlutusTxOut . snd) . sortOn fst . UTxO.pairs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
map snd . sortOn fst . UTxO.pairs
should be equivalent to toList
@@ -79,5 +77,5 @@ newLedgerEnv pparams = | |||
-- both unused in Hydra. There might be room for interesting features in the | |||
-- future with these two but for now, we'll consider them empty. | |||
Ledger.ledgerAccount = Ledger.AccountState mempty mempty | |||
, Ledger.ledgerPp = toLedgerPParams ShelleyBasedEraAlonzo pparams | |||
, Ledger.ledgerPp = toLedgerPParams ShelleyBasedEraBabbage pparams |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make sure to also adjust the sample protocol parameters we have to be Babbage's params instead of Alonzo (i.e. remove d
and extraEntropy
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the included protocol-parameters.json with ones from the vasil testnet (also has updated cost models).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the one retrieved from the vasil testnet using the cardano-cli did include d and entropy as null
fields:
[...]
"decentralization": null,
"executionUnitPrices": {
"priceMemory": 5.77e-2,
"priceSteps": 7.21e-5
},
"extraPraosEntropy": null,
[...]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, because the cardano-cli / cardano-api have only one data-type to represent both Alonzo and Babbage params, with Maybe
values 😬 ...
8994ef0
to
1b1819f
Compare
Transactions CostsSizes and execution budgets for Hydra protocol transactions. Note that unlisted parameters are currently using
Cost of Init Transaction
Cost of Commit TransactionCurrently only one UTxO per commit allowed (this is about to change soon)
Cost of CollectCom Transaction
Cost of Close Transaction
Cost of Contest Transaction
Cost of Abort Transaction
Cost of FanOut TransactionInvolves spending head output and burning head tokens. Uses ada-only UTxO for better comparability.
|
Unit Test Results234 tests +4 232 ✔️ +4 12m 37s ⏱️ + 3m 41s Results for commit 254cb9c. ± Comparison against base commit 799da77. This pull request removes 19 and adds 23 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments on recent changes
, "datum" .= Null | ||
, "datumhash" .= Null | ||
, "inlineDatum" .= Null | ||
, "referenceScript" .= Null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😮 Is this our life now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf commit message: 13626d5
alicesCallback `shouldSatisfyInTime` \case | ||
Observation OnCloseTx{snapshotNumber, remainingContestationPeriod} -> | ||
-- FIXME(SN): should assert contestationDeadline > current | ||
Just (snapshotNumber == 1, remainingContestationPeriod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Maybe (Bool, b)
is a bit of an odd type.. how about Maybe b
and do a guard $ snapshotNumber == 1
here?
where | ||
-- NOTE: Somehow, since 1.35.0, cleaning-up cardano-node database directory | ||
-- _sometimes_ generates an empty 'clean' file which prevents the 'db' folder | ||
-- to be fully removed and triggers an 'UnsatisfiedConstraints' IOException. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an odd workaround and refers to the cardano-node
behavior, while this function is very generic and lives in hydra-test-utils
. Can't we do some node-specific cleanup prior to this in the action
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. If you feel like debugging this, go ahead. Spent already an hour chasing this thing and gave up.
hydra-cluster/src/CardanoNode.hs
Outdated
kesKeyFilename i = | ||
dirname i </> "kes.skey" | ||
opCertFilename i = | ||
dirname i </> "opcert.cert" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can get rid of all those i
s by simply defining dirname = "stake-pool-" <> show (nodeId cfg)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this myself.
Mostly cardano-node and consensus from open PRs + plutus a bit further ahead on release/1.0.0 to get serialiseData
This is mostly updating TxOut, TxOutDatum and TxBodyContent patterns.
We picked some interesting periods like day, week etc. as well as the horizon when a transaction is definitely final on mainnet (after k blocks)
This allows us again to get hold of the comittedUTxO for a counterexample
This makes it individual committedUTxO explicit. In some cases the individual entries "collapsed" because of identical TxIn values.
For some reason the Arbitrary instance for Ledger.TxIn seems to have degraded in likelihood of collisions, but Arbitrary instance on the Cardano.Api.TxIn has not (we defined that one).
They need to be collision resistant (stronger) or do not generate "collapsing" foldable values (weaker).
This makes us depend (hopefullly) temporarily on a fork of cardano-node that fixes ToJSON insteance for ledger scripts.
Also drop Num instance for ContestationPeriod and instead define a fixture 'cperiod'.
Dropped that function and module because we do not intend to run multiple cardano nodes, but instead a single block producer holding all the stake going forward. We separated the things contained into Hydra.Cluster.{Faucet,Fixture,Util}.
No more BFT nodes, so we now fork into the last era on 'epoch 0' and run a single stake pool.
We would like to use the cardano-ledger's generator to test our internal ledger behaves correctly but this is currently problematic because it's hard to tweak the generation process to produce only the kind of txs we want in a Head. Given we are only using those generators for "smoke" testing the Ledger's validation function over a sequence of txs, it's probably fine but we should revisit this in the future.
The node is no longer a BFT one though, will rename eventually.
This is needed until we leverage reference scripts and inputs.
Somehow... cardano-node database now include an empty file 'clean', of 0 byte, which doesn't gets deleted (or is maybe re-created?) and causes the 'removePathForcibly' function to raise an awkward exception: removeDirectory: unsatisfied constraints (Directory not empty) despite having deleted everything else. If we inspect temporary directories after the exception is raised, they look like the following: run/user/1000/hydra-cluster-b5c7621dbb5c60a1 └── db └── clean With that 'clean' file being empty and of size 0. :shrugging-man:
Instead of computing a delay from hard-coded parameters and error-prone equation. This was now apparently triggered too fast? Maybe the computation was bonkers from the start and we didn't noticed somehow because BFT nodes took longer to process the transaction? This is more correct anyway so, good to do.
I still don't like this instance. It's cumbersome, inconsistent and ugly. I wish we also had more control over our JSON API, there's too much things that can break because we update cardano-api at the moment.
Also left a FIXME about the wrong promises made by the function name and its arguments.
It was a team effort.. no approval necessary ;) |
Follow up of #355
As described in #406 the goal of this PR is to make all packages compile and target Babbage and PlutusV2 everywhere. From there we can start fixing the tests and make this green. Besides "smaller" issues in the
hydra-node
tests, bigger tasks are listed in #400.